"close others" extension added to default extensions directory. #4590

sathyamoorthi · 2013-07-28T16:46:46Z

sathyamoorthi · 2013-07-29T01:32:29Z

I got three warnings in "Travis Build" and notified as Failed. I gave proper pull request. But i don't know the reason for Failure. Can anybody help me in this to proceed further?

larz0 · 2013-07-29T03:39:25Z

@sathyamoorthi I've been getting those warnings as well. I'll mention it tomorrow.

sathyamoorthi · 2013-07-29T04:36:17Z

@larz0 ahhh. Thank you. I am not an expert in Github. I created this PR because i got this same error in my old PR. When i got "Travis" warnings again here, i was totally blank. Thank god, you gave positive reply.

JeffryBooher · 2013-07-29T22:16:07Z

@sathyamoorthi There was a change to jsHint that is causing all builds to fail. We have a fix that will go in once we have sprint 28 wrapped up and delivered.

JeffryBooher · 2013-07-29T22:18:28Z

src/extensions/default/CloseOthers/settings.json

@@ -0,0 +1,5 @@
+{
+    "close_others": true,
+    "close_above": false,


We should default these to true or users will not discover them. The ideal way is to turn them on by default and let users turn them off.

@JeffryBooher Is there a way to add and remove context menus (close others above & close others below) at runtime?

Since the menus are created in/from JavaScript after the shell launches, they are all created at runtime. You should be able to add/remove/disable the menu items at any time.

sathyamoorthi · 2013-07-30T06:09:11Z

@JeffryBooher small bug in this pull request. Let me update this on next commit.

sathyamoorthi · 2013-07-31T07:06:14Z

@JeffryBooher, @TomMalbran committed new set of changes that you both mentioned. Still, i am expecting some more changes after your review. So i didn't add comments to functions that i have added in the core. Once, everything finalized we can add function headers.

JeffryBooher · 2013-08-25T02:23:14Z

@sathyamoorthi are you still working on this?

sathyamoorthi · 2013-08-25T02:35:35Z

@JeffryBooher yes. i committed changes that you have mentioned. i'm waiting for your review.

JeffryBooher · 2013-08-25T03:03:38Z

src/document/DocumentManager.js

-     */
-    function removeFromWorkingSet(file, suppressRedraw) {
+
+    function _removeFromArrays(file) {


this name isn't very descriptive plus it isn't changing the behavior. Maybe _internalRemoveFromWorkingSet() is a better name?

JeffryBooher · 2013-08-25T03:14:06Z

Done with review. Just a couple of minor naming nits and this is ready to merge. Good Job!

sathyamoorthi · 2013-08-25T03:21:44Z

src/extensions/default/CloseOthers/settings.json

+    "close_others": true,
+    "close_above": true,
+    "close_below": true
+}


@JeffryBooher Shall we enable only "close others"? Because, it would annoy users by showing other two (close_above and close_below) all the time. If i once know how to add and remove menus dynamically we can add them. what do you think?

Yep, I say show them. If it becomes a problem we can easily turn it off

Yes. consider if there is only one file in the working set, it will still show "Close Others", "Close Others Above" and "Close Others Below". So i should work next to add and remove menus intelligently based user click. So if you wish, i can down those two flags and give pull request for now.

I considered it and the convention is to disable the menu items which we have a story for and we will be working soon. So leave them enabled for now and then we'll have to fix it so they are disabled when the first/last item are selected later.

we have many menu items that are enabled when they shouldn't be so we are aware of the problem.

oh. that's cool. just making sure you know this nit. then, no problem. let me do those small change and give PR.

sathyamoorthi · 2013-08-25T04:28:30Z

@JeffryBooher changed those function names.

JeffryBooher · 2013-08-27T04:09:49Z

src/document/DocumentManager.js

+        return true;
+    }
+
+    function removeFromWorkingSet(file, suppressRedraw) {


minor nit: this should be named removeFileFromWorkingSet()

actually what would be better is if you could pass a string or an array to removeFromWorkingSet(). you can use $.isArray() to determine if it's an array then you don't need two different functions.

I think you could re-work the handlers for workingSetRemove so there isn't 2 different events (workingSetRemove and workingSetRemoveList) using the same technique.

We can also use the new EcmaScript 5 Array.isArray() function.

In project/WorkingSetView.js see _handleFileRemoved function. What is the need of below code.

// Make the next file in the list show the close icon, // without having to move the mouse, if there is a next file. var $nextListItem = $listItem.next(); if ($nextListItem && $nextListItem.length > 0) { var canClose = ($listItem.find(".can-close").length === 1); var isDirty = isOpenAndDirty($nextListItem.data(_FILE_KEY)); _updateFileStatusIcon($nextListItem, isDirty, canClose); }

In comments it stated that it is used to show icon on the next file. But i removed this block and still i'm able to see icon on the next file.

I don't think you want to remove this block. the icon shows up on mouse over so this is just to make sure that when you click the 'x' that it shows up when the item below moves up.

JeffryBooher · 2013-08-27T16:58:40Z

Can you merge master into this branch? There are quite a number of unit-test failures that I don't understand. I'm hoping that merging master into the branch fixes most of them.

sathyamoorthi · 2013-08-31T02:26:26Z

@JeffryBooher any more changes needed?

JeffryBooher · 2013-08-31T02:29:52Z

@sathyamoorthi I haven't had time the past few days to look at it since we've been closing down Sprint 30. I'll look at it over the weekend.

sathyamoorthi · 2013-08-31T02:41:08Z

@JeffryBooher sure.np.

JeffryBooher · 2013-09-03T16:55:31Z

The code looks great.

There are a few unit test failures that you need to fix plus you need unit tests for close others, close above, and close below.

I'd start by merging master into your branch. I think there are some failures that may be due to the fact that you're out of date with the current shell plus some others that have been fixed in master.

sathyamoorthi · 2013-09-11T17:30:19Z

@JeffryBooher I have added unit tests for close others, above and below. Please take a look.

JeffryBooher · 2013-09-17T23:55:55Z

@sathyamoorthi looks like you forgot to add the "unittest-files" folder and the unit test files to your pull request :)

sathyamoorthi · 2013-09-18T04:58:57Z

@JeffryBooher Hi. Actually unittest-files folder don't have any content. Files will be created at run time inside that folder and will be deleted once everything finished well. So, to commit that folder i have added a dummy file.

And i'm seeing lot of integration test failures. I'm not sure whether i broke anything by modifying DocumentCommandHandlers and DocumentManager.

JeffryBooher · 2013-09-18T05:20:14Z

@sathyamoorthi you might want to merge master into your branch snd see if that fixes the test failures.

sathyamoorthi · 2013-09-18T05:22:47Z

@JeffryBooher I merged master into my branch. But still i see failures. Meanwhile, no errors when i run tests directly on Master branch.

JeffryBooher · 2013-09-18T05:27:29Z

I can take a look

sathyamoorthi · 2013-09-18T05:28:26Z

great. thanks.

peterflynn · 2013-09-24T23:38:34Z

src/document/DocumentManager.js

+        return true;
+    }
+
+    function removeFromWorkingSet(content, suppressRedraw) {


All the docs for this API were lost -- please copy them from the diff above. (And update the type annotation -- I think @param {!FileEntry|Array.<FileEntry>} file ?)

JeffryBooher · 2013-09-24T23:46:59Z

@sathyamoorthi thanks for the heads up. This turned out to be an async issue in the cleanup temp folder unit test helper. We may still have some issues with that but it looks like @jasonsanjose fixed it in before the end of the sprint last week.

I merged master into your branch and tried it again and the tests are still failing. I'm not sure why except that there are a few tests that fail in your branch that are caused by changes you made to saving and the dirty flag.

Merge master into your branch and fix the Document and Document Command Handlers integration test failures and we will go from there. I suspect that the API changes you made may be impacting these tests.

Hopefully the other tests will fix themselves with the merge from master.

sathyamoorthi · 2013-10-02T11:39:18Z

@JeffryBooher Fixed all integration test failures. Please do another review.

JeffryBooher · 2013-10-02T20:32:58Z

@sathyamoorthi thanks. Unfortunately it's too late to take for Sprint 32 I'll nominate this for Sprint 33 if the tests pass.

sathyamoorthi · 2013-10-03T04:53:45Z

@JeffryBooher sure. no problem. Let me know, if it needs any more changes.

JeffryBooher · 2013-10-09T16:33:33Z

Looks good. All tests pass. Merging! Thanks @sathyamoorthi!

"close others" extension added to default extensions directory.

sathyamoorthi · 2013-10-10T04:39:35Z

@JeffryBooher Wow ! Thanks for pulled it in. Let me know if it needs any change at any time in future.

"close others" extension.

d36c1e9

sathyamoorthi mentioned this pull request Jul 29, 2013

[OPEN] "close others" extension added to default extensions directory. #4469

Closed

JeffryBooher reviewed Jul 29, 2013
View reviewed changes

ghost assigned JeffryBooher Jul 30, 2013

file close functions to handle list of files.

01c33fb

JeffryBooher reviewed Aug 25, 2013
View reviewed changes

sathyamoorthi reviewed Aug 25, 2013
View reviewed changes

naming convention changes

e8fae97

JeffryBooher reviewed Aug 27, 2013
View reviewed changes

sathyamoorthi added 2 commits August 28, 2013 11:18

Merge branch 'master' into closeOthers2

540e140

removeFromWrokingSet() changed to handle array of files.

9e412f0

jsHint fixes

9086c1d

unnittest-files folder added.

5f2c809

Merge branch 'master' into closeOthers2

024b654

peterflynn reviewed Sep 24, 2013
View reviewed changes

sathyamoorthi added 3 commits October 1, 2013 16:45

Merge branch 'master' into closeOthers2

841a923

Merge branch 'master' into closeOthers2

967738e

Integration test failures fixed

bd4018d

JeffryBooher added a commit that referenced this pull request Oct 9, 2013

Merge pull request #4590 from sathyamoorthi/closeOthers2

b6e786b

"close others" extension added to default extensions directory.

JeffryBooher merged commit b6e786b into adobe:master Oct 9, 2013

sathyamoorthi mentioned this pull request Oct 12, 2013

Close Others extension improvements. #5497

Closed

peterflynn mentioned this pull request Nov 1, 2013

Nothing open in editor area after using Close Others #5800

Closed

sathyamoorthi mentioned this pull request Nov 6, 2013

Fix for issue #5800 #5866

Closed

"close others" extension added to default extensions directory. #4590

"close others" extension added to default extensions directory. #4590

Conversation

sathyamoorthi commented Jul 28, 2013

sathyamoorthi commented Jul 29, 2013

larz0 commented Jul 29, 2013

sathyamoorthi commented Jul 29, 2013

JeffryBooher commented Jul 29, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sathyamoorthi commented Jul 30, 2013

sathyamoorthi commented Jul 31, 2013

JeffryBooher commented Aug 25, 2013

sathyamoorthi commented Aug 25, 2013

Choose a reason for hiding this comment

JeffryBooher commented Aug 25, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sathyamoorthi commented Aug 25, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JeffryBooher commented Aug 27, 2013

sathyamoorthi commented Aug 31, 2013

JeffryBooher commented Aug 31, 2013

sathyamoorthi commented Aug 31, 2013

JeffryBooher commented Sep 3, 2013

sathyamoorthi commented Sep 11, 2013

JeffryBooher commented Sep 17, 2013

sathyamoorthi commented Sep 18, 2013

JeffryBooher commented Sep 18, 2013

sathyamoorthi commented Sep 18, 2013

JeffryBooher commented Sep 18, 2013

sathyamoorthi commented Sep 18, 2013

Choose a reason for hiding this comment

JeffryBooher commented Sep 24, 2013

sathyamoorthi commented Oct 2, 2013

JeffryBooher commented Oct 2, 2013

sathyamoorthi commented Oct 3, 2013

JeffryBooher commented Oct 9, 2013

sathyamoorthi commented Oct 10, 2013